Conversation
|
|
||
| {%- block build %} | ||
| {%- if 'ccache-toolchain' in ' '.join(package['meta']['requirements']['build']) %} | ||
| conda build --no-build-id /recipe_root --quiet || exit 1 |
There was a problem hiding this comment.
Please leave a comment explaining why --no-build-id is being used. Something like:
ccache uses the compiler command line to generate the cache keys, so we can't have a different path for every build.
ccache provides CCACHE_BASEDIR variable to solve this but it's better to avoid setting it as it has some side effects.
|
|
||
| {%- if 'ccache-toolchain' in ' '.join(package['meta']['requirements']['build']) %} | ||
| cache: | ||
| branch: no-md5deep |
There was a problem hiding this comment.
This branch should not be needed anymore: travis-ci/travis-ci#7456 (comment)
However I didn't test it after they fixed it. After you make the changes I can help with testing before merging this.
| {%- if matrix %} | ||
|
|
||
| {%- if 'ccache-toolchain' in ' '.join(package['meta']['requirements']['build']) %} | ||
| - if test -f ./build_failed; then exit 1; fi |
There was a problem hiding this comment.
It would be better to echo a message before exiting:
if test -f ./build_failed; echo "Build step failed. Please look at the exact error message above"; then exit 1; fi
|
|
||
| script: | ||
| {%- if 'ccache-toolchain' in ' '.join(package['meta']['requirements']['build']) %} | ||
| - conda build --no-build-id ./{{ recipe_dir }} |
There was a problem hiding this comment.
Same as above, please add a comment explaining --no-build-id
| - docker pull condaforge/linux-anvil | ||
|
|
||
| {%- if matrix %} | ||
| {%- if 'ccache-toolchain' in ' '.join(package['meta']['requirements']['build']) %} |
There was a problem hiding this comment.
Just thinking about what would be done if we decide to use ccache by default.
We would move ccache-toolchain code to toolchain. Then this if would check for toolchain package instead, do you agree?
Also we could use conda-forge.yml for that. It would be more explicit and make it possible to disable ccache stuff altogether.
There was a problem hiding this comment.
Yes, if we moved the code to toolchain then this if check would check for toolchain.
I had it this way, so that we can disable caching in different platforms simply by using a selector on ccache-toolchain.
| {%- if matrix %} | ||
| {%- if 'ccache-toolchain' in ' '.join(package['meta']['requirements']['build']) %} | ||
| # Run, test and (if we have a BINSTAR_TOKEN) upload the distributions. | ||
| - ./ci_support/run_docker_build.sh || touch ./build_failed |
There was a problem hiding this comment.
A comment here justifying this would be helpful too: "If any command present on dependencies section fails, CircleCI doesn't cache anything. That's why we need to check for failure later"
|
I'm testing with llvmdev feedstock and it seems The build phase respects the option, but the test phase does not: It tries to delete the work dir with id. As a consequence, the test phase keeps the real work dir and then the next package build breaks because it tries to apply a patch it was already applied. On Travis it is working fine because each package is built on separate jobs |
|
Could you please raise that as an issue on the |
|
Looks easy enough to fix. This line https://github.com/conda/conda-build/blob/2.1.x/conda_build/build.py#L1279 needs to fixed as in https://github.com/conda/conda-build/blob/2.1.x/conda_build/build.py#L1491 |
|
...or PR. This should be less of an issue when we have a proper CircleCI matrix, which will be sooner rather than later given recent changes. xref: #203 (comment) |
|
So I was in the process of trying to upgrade to CircleCI 2.0. It looks like this ends up being a pretty serious overhaul. Maybe we should pause this until that and matrix support is in. Thoughts? xref: #530 |
|
So we need to check how the cache behaves with CircleCI 2.0. |
|
Yeah, the linked PR already passes and is ready to merge. Would like to play with setting up matrix builds as a follow up PR. |
|
Also I would like to resurrect the package caching work that @gqmelo did. My initial thinking is that caching of packages should always happen (so not be a setting people can change). I think by having a caching step that always happens will simplify the logic here. |
|
Ready for review now |
|
LGTM Have you tested with an existing feedstock? |
|
@gqmelo, I haven't with the latest changes. I'll check and get back to you |
|
Removing from 3.0.0 milestone as this is not yet ready |
3 participants
I've kept the template unchanged when
ccache-toolchainis not a build dependency. I can simplify the code a bit more, but that means the template will change.cc @gqmelo